Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(profiler): data race accessing s.server #15

Merged
merged 3 commits into from
Feb 19, 2024
Merged

fix(profiler): data race accessing s.server #15

merged 3 commits into from
Feb 19, 2024

Conversation

c3y1huang
Copy link
Contributor

Which issue(s) this PR fixes:

Issue N/A

What this PR does / why we need it:

Fix the concurrent access and modification of the s.server.

Special notes for your reviewer:

Additional documentation or context

This is noticed in #12 (comment), and detected by the unit testing.

=== RUN   Test
time="2024-02-02T01:00:24Z" level=info msg="Profiler operation: SHOW, port: 0"
time="2024-02-02T01:00:24Z" level=info msg="Preparing to show the profiler address"
time="2024-02-02T01:00:24Z" level=info msg="Profiler operation: ENABLE, port: 55555"
time="2024-02-02T01:00:24Z" level=info msg="Prepareing to enable the profiler"
time="2024-02-02T01:00:24Z" level=info msg="Waiting the profiler server(:55555) to start"
time="2024-02-02T01:00:24Z" level=info msg="Profiler operation: DISABLE, port: 55555"
time="2024-02-02T01:00:24Z" level=info msg="Preparing to disable the profiler"
time="2024-02-02T01:00:24Z" level=info msg="Profiler server (:55555) is closed"
==================
WARNING: DATA RACE
Write at 0x00c0000ae0a0 by goroutine 34:
  github.com/longhorn/go-common-libs/profiler.(*Server).DisableProfiler()
      /go-common-libs/profiler/profiler.go:218 +0x3fb
  github.com/longhorn/go-common-libs/profiler.(*Server).ProfilerOP()
      /go-common-libs/profiler/profiler.go:123 +0x229
  github.com/longhorn/go-common-libs/generated/profilerpb._Profiler_ProfilerOP_Handler()
      /go-common-libs/generated/profilerpb/profiler_grpc.pb.go:79 +0x22d
  google.golang.org/grpc.(*Server).processUnaryRPC()
      /go-common-libs/vendor/google.golang.org/grpc/server.go:1372 +0x196b
  google.golang.org/grpc.(*Server).handleStream()
      /go-common-libs/vendor/google.golang.org/grpc/server.go:1783 +0x1a31
  google.golang.org/grpc.(*Server).serveStreams.func2.1()
      /go-common-libs/vendor/google.golang.org/grpc/server.go:1016 +0xd2

Previous read at 0x00c0000ae0a0 by goroutine 28:
  github.com/longhorn/go-common-libs/profiler.(*Server).EnableProfiler.func1()
      /go-common-libs/profiler/profiler.go:178 +0x2aa

Goroutine 34 (running) created at:
  google.golang.org/grpc.(*Server).serveStreams.func2()
      /go-common-libs/vendor/google.golang.org/grpc/server.go:1027 +0x204
  google.golang.org/grpc/internal/transport.(*http2Server).operateHeaders()
      /go-common-libs/vendor/google.golang.org/grpc/internal/transport/http2_server.go:603 +0x3a01
  google.golang.org/grpc/internal/transport.(*http2Server).HandleStreams()
      /go-common-libs/vendor/google.golang.org/grpc/internal/transport/http2_server.go:648 +0x285
  google.golang.org/grpc.(*Server).serveStreams()
      /go-common-libs/vendor/google.golang.org/grpc/server.go:1012 +0x6bb
  google.golang.org/grpc.(*Server).handleRawConn.func1()
      /go-common-libs/vendor/google.golang.org/grpc/server.go:939 +0x86

Goroutine 28 (finished) created at:
  github.com/longhorn/go-common-libs/profiler.(*Server).EnableProfiler()
      /go-common-libs/profiler/profiler.go:171 +0x40a
  github.com/longhorn/go-common-libs/profiler.(*Server).ProfilerOP()
      /go-common-libs/profiler/profiler.go:119 +0x3aa
  github.com/longhorn/go-common-libs/generated/profilerpb._Profiler_ProfilerOP_Handler()
      /go-common-libs/generated/profilerpb/profiler_grpc.pb.go:79 +0x22d
  google.golang.org/grpc.(*Server).processUnaryRPC()
      /go-common-libs/vendor/google.golang.org/grpc/server.go:1372 +0x196b
  google.golang.org/grpc.(*Server).handleStream()
      /go-common-libs/vendor/google.golang.org/grpc/server.go:1783 +0x1a31
  google.golang.org/grpc.(*Server).serveStreams.func2.1()
      /go-common-libs/vendor/google.golang.org/grpc/server.go:1016 +0xd2
==================
PASS: <autogenerated>:1: TestSuite.TestProfilerServiceOperations        0.006s
OK: 1 passed
    testing.go:1465: race detected during execution of test
--- FAIL: Test (0.01s)
=== NAME  
    testing.go:1465: race detected during execution of test
FAIL
coverage: 76.6% of statements
FAIL    github.com/longhorn/go-common-libs/profiler     0.024s

@c3y1huang c3y1huang self-assigned this Feb 2, 2024
@c3y1huang c3y1huang requested review from Vicente-Cheng and a team February 2, 2024 02:11
@c3y1huang c3y1huang marked this pull request as ready for review February 2, 2024 02:12
Vicente-Cheng
Vicente-Cheng previously approved these changes Feb 2, 2024
View reviewed changes
Copy link
Contributor

@Vicente-Cheng Vicente-Cheng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch! thanks

@c3y1huang c3y1huang enabled auto-merge (rebase) February 2, 2024 03:07
shuo-wu
shuo-wu reviewed Feb 2, 2024
View reviewed changes
profiler/profiler.go Outdated Show resolved Hide resolved
@c3y1huang c3y1huang force-pushed the fix-unit-test-data-race branch 2 times, most recently from 6f3aa84 to 4387242 Compare February 2, 2024 03:41
shuo-wu
shuo-wu previously approved these changes Feb 2, 2024
View reviewed changes
@c3y1huang c3y1huang force-pushed the fix-unit-test-data-race branch 2 times, most recently from 28a50e3 to 0edb0d5 Compare February 2, 2024 03:49
Vicente-Cheng
Vicente-Cheng previously approved these changes Feb 2, 2024
View reviewed changes
Copy link
Contributor

@Vicente-Cheng Vicente-Cheng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for handling the timeout error!

@c3y1huang c3y1huang enabled auto-merge (rebase) February 2, 2024 05:10
@c3y1huang c3y1huang mentioned this pull request Feb 2, 2024
Merged
shuo-wu
shuo-wu previously approved these changes Feb 2, 2024
View reviewed changes
@c3y1huang
fix: typo
51d4b89 
Signed-off-by: Chin-Ya Huang <[email protected]>
@c3y1huang c3y1huang dismissed stale reviews from shuo-wu and Vicente-Cheng via 51d4b89 February 19, 2024 00:54
@innobead innobead merged commit 4b3c094 into longhorn:main Feb 19, 2024
3 checks passed
@c3y1huang c3y1huang deleted the fix-unit-test-data-race branch March 1, 2024 04:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@innobead innobead innobead approved these changes

@shuo-wu shuo-wu shuo-wu left review comments

@Vicente-Cheng Vicente-Cheng Vicente-Cheng left review comments

@derekbit derekbit Awaiting requested review from derekbit

Assignees

@c3y1huang c3y1huang

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@c3y1huang @innobead @Vicente-Cheng @shuo-wu